Generally processing of exception with serial execution. #848

boaks · 2019-01-11T12:42:52Z

Processes exceptions within the serialized execution consequently.
Forward failures on block request to the outer request.

Signed-off-by: Achim Kraus [email protected]

boaks · 2019-01-11T12:44:32Z

I detected some issues with exception and not forwarded failure during PR #844 .
I'm not sure, if we should include that on M13. We can merge it also later.

sbernard31

This modification make me think about that old one : cede7b1
In this commit, I added try catch on AbstractLayer too.

About catching Exception, I think maybe this is better to catch RuntimeException because this doesn't hide future addition of checked exception. (this is just a details)

sbernard31 · 2019-01-11T16:38:39Z

californium-core/src/main/java/org/eclipse/californium/core/network/stack/BaseCoapStack.java

+				// no longer an observe option. Therefore check the
+				// request for it.
+				exchange.retransmitResponse();
+			}


This is totally out of topic because you just move this if block from CoapEndpoint to BaseCoapStack but I can not get the point of it.

I move it into the "try - catch". The CoapEndpoint, for some reasons, use

if (exchange.checkOwner()) { // send response while processing exchange. coapstack.sendResponse(exchange, response); } else { exchange.execute(new Runnable() { @Override public void run() { coapstack.sendResponse(exchange, response); } }); }

In my very first variant, I used two try-catch, but moving it reduces that. And moving the if into the try-catch makes it somehow more complete.

The question was more about the code itself (exchange.retransmitResponse) instead of the move. (that's why I notice that my question was totally out of topic :p)

OK. A server-side-observe-exchange gets completed with the current response. Before sending the next notify, the Exchange must be prepared to accept that.

Ok It's a bit clearer
(even If I do not really understand how we do that as the function just set the exchange as "not complete".

At first sight, I was confused because I thought that retransmitResponse does "retransmit the previous response)

sbernard31 · 2019-01-11T16:43:04Z

californium-core/src/main/java/org/eclipse/californium/core/network/UdpMatcher.java

@@ -295,7 +305,7 @@ public void run() {
 									return;
 								}
 							} catch (Exception ex) {
-								LOGGER.error("error receiving response {} for {}", response, prev, ex);
+								LOGGER.debug("error receiving response {} for {}", response, prev, ex);


Those 3 catch (Exception ex) logged in DEBUG and now in ERROR.

Intuitively I would have let it in ERROR. Any reason about this change ?

My intention was, that something received, which is broken, floods the log.
But I can go back to error, if you prefer.

Is "warn" OK?

My intention was, that something received, which is broken, floods the log.

I get the point, but in the same time, this will be hard to detect unexpected error if we let it in debug. So "warn" sounds reasonable.

sbernard31 · 2019-01-11T16:58:36Z

...ium-core/src/main/java/org/eclipse/californium/core/network/stack/Block1BlockwiseStatus.java

 		block.addMessageObservers(request.getMessageObservers());
+		block.addMessageObserver(new FailureForwardingMessageObserver(request));


This will not send event twice ?
one for : block.addMessageObservers(request.getMessageObservers());
and one for : block.addMessageObserver(new FailureForwardingMessageObserver(request));

May be. Without it, a RST for a inner request "completes" the exchange, but the origin Request will never receive the reject.
(I found that, when the "assemble request" failed and rejected the last block request. The test show, that the exchange completes, but the test waits ...)

FMPOV, solving that moves this PR out of M13.

In the past, the relation of inner and outer request was one of the pitfalls, which caused leaks. So I'm not sure, how much testing is needed, if it's changed more, to support "application-layer MessageObserver" as well.

Or split that "forwarding" from this PR into a new PR, merge this for M13, and solve the "forwarding" later.

I feel that block.addMessageObservers(request.getMessageObservers()); was not done for users but for house-keeping.

Maybe if you now add new FailureForwardingMessageObserver(request) which forwards error to the main request. the older block.addMessageObservers(request.getMessageObservers()); could be removed ?

But maybe we take the problem is the wrong way and we should ask : How we want this behave ?

Let's split this PR after M13 and move the forwarding discussion to issue #818

sbernard31 · 2019-01-11T17:03:23Z

californium-core/src/test/java/org/eclipse/californium/core/test/Issue834Test.java

+ *         testFetchWithLargePayload(nl.teslanet.test.cf.Issue834Test): wrong
+ *         content length returned: expected:<8192> but was:<2012> Tests run:
+ *         12, Failures: 4, Errors: 0, Skipped: 0
+ */


Is it intended to add this tests class in this PR ?

Uups, I asked for that to be contributed by the author. It's "on the way", I remove it here.

It's now PR #849

sbernard31 · 2019-01-11T17:36:46Z

californium-core/src/main/java/org/eclipse/californium/core/network/UdpMatcher.java

+						receiver.receiveRequest(previous, request);
+					} catch (Exception ex) {
+						LOGGER.debug("error receiving request {} again!", request, ex);
+						receiver.reject(request);


I'm not sure if sending RST is a good idea... 🤔

Did you consider to send an Internal Server Error Response ?

I added those try-catch, because without, nothing happens.
For a "Internal Server Error Response" I would try to create more speaky ones.
If a Exception reaches that, I hope the log will help to fix the bug or create a speaky response.

boaks · 2019-01-11T18:29:34Z

In this commit, I added try catch on AbstractLayer too.

There it will traverse more try-catches, but yes, generally, it's similar.

About catching Exception, I think maybe this is better to catch RuntimeException because this doesn't hide future addition of checked exception. (this is just a details)

Makes sense. FMPOV you raise some question, which will take some more time to find better solutions. So, I would move that after M13. What do you think?

boaks · 2019-01-12T07:28:24Z

About catching Exception, I think maybe this is better to catch RuntimeException because this doesn't hide future addition of checked exception. (this is just a details)

Switched to RuntimeException.

sbernard31 · 2019-01-14T15:05:45Z

So, I would move that after M13. What do you think?

👍

boaks · 2019-01-14T15:54:08Z

Move the complete PR after M13 or split the forwarding and move that after M13 ?

sbernard31 · 2019-01-14T16:22:52Z

I thought that you finally propose to complete the PR after the M13. So my 👍 was for this.

But if you need this for M13 and want to split the forwarding. If have no objection.

boaks · 2019-01-14T16:28:39Z

The "finally propose" was in the timeline before the propose to split it. But yes, move the complete PR after M13.

Signed-off-by: Achim Kraus <[email protected]>

boaks · 2019-01-21T17:12:23Z

I split the failure forwarding. Let's try to discus this ahead of solving it in issue #818

sbernard31 · 2019-01-22T11:00:57Z

Except the block part for which I have some concern and for which we will discuss in #818.

LGTM

boaks · 2019-01-22T11:34:53Z

Except the block part

There are two block parts, the one is the try catch around the "next block request", the other was the removed failure forwarding. Do you refer only to the failure forwarding?

sbernard31 · 2019-01-22T13:10:22Z

Do you refer only to the failure forwarding?

Yes, only this part.

sbernard31 reviewed Jan 11, 2019

View reviewed changes

boaks force-pushed the serialexecution_exceptions branch from a94f267 to a40cb10 Compare January 12, 2019 07:27

boaks force-pushed the serialexecution_exceptions branch from a40cb10 to fe36f57 Compare January 12, 2019 07:29

boaks mentioned this pull request Jan 14, 2019

setReadyToSent call several times with transparent block mode. #818

Open

Generally processing of exception with serial execution.

a87c5e8

Signed-off-by: Achim Kraus <[email protected]>

boaks force-pushed the serialexecution_exceptions branch from fe36f57 to a87c5e8 Compare January 21, 2019 17:10

boaks merged commit a99948e into eclipse-californium:2.0.x Jan 22, 2019

boaks deleted the serialexecution_exceptions branch April 5, 2019 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generally processing of exception with serial execution. #848

Generally processing of exception with serial execution. #848

boaks commented Jan 11, 2019

boaks commented Jan 11, 2019

sbernard31 left a comment

sbernard31 Jan 11, 2019

boaks Jan 11, 2019 •

edited

Loading

sbernard31 Jan 14, 2019

boaks Jan 14, 2019

sbernard31 Jan 14, 2019

sbernard31 Jan 11, 2019

boaks Jan 11, 2019

boaks Jan 12, 2019

sbernard31 Jan 14, 2019

sbernard31 Jan 11, 2019

boaks Jan 11, 2019

boaks Jan 12, 2019

boaks Jan 12, 2019

sbernard31 Jan 14, 2019

boaks Jan 17, 2019

sbernard31 Jan 11, 2019

boaks Jan 11, 2019

boaks Jan 12, 2019

boaks Jan 14, 2019

sbernard31 Jan 11, 2019

boaks Jan 11, 2019

boaks commented Jan 11, 2019

boaks commented Jan 12, 2019

sbernard31 commented Jan 14, 2019

boaks commented Jan 14, 2019

sbernard31 commented Jan 14, 2019

boaks commented Jan 14, 2019

boaks commented Jan 21, 2019

sbernard31 commented Jan 22, 2019

boaks commented Jan 22, 2019

sbernard31 commented Jan 22, 2019

		block.addMessageObservers(request.getMessageObservers());
		block.addMessageObserver(new FailureForwardingMessageObserver(request));

Generally processing of exception with serial execution. #848

Generally processing of exception with serial execution. #848

Conversation

boaks commented Jan 11, 2019

boaks commented Jan 11, 2019

sbernard31 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaks Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boaks commented Jan 11, 2019

boaks commented Jan 12, 2019

sbernard31 commented Jan 14, 2019

boaks commented Jan 14, 2019

sbernard31 commented Jan 14, 2019

boaks commented Jan 14, 2019

boaks commented Jan 21, 2019

sbernard31 commented Jan 22, 2019

boaks commented Jan 22, 2019

sbernard31 commented Jan 22, 2019

boaks Jan 11, 2019 •

edited

Loading